shell: Even more types #21425

mvollmer · 2024-12-12T07:00:31Z

No description provided.

A manifest is now just a JsonObject, and users are expected to type-cast it (or parts) of it to more concrete types, like the new ShellManifest and ManifestParentSection.

The cockpit.spawn function takes only two arguments (unlike cockpit.script). Location.reload() doesn't take any arguments, the "force_reload" argument is a Firefox extension.

mvollmer · 2024-12-12T07:01:26Z

This time I tried out fully typing two largish files without any intermediate "relaxed" steps. It's fun but also quite intense.

cockpituous · 2024-12-12T08:07:05Z

pkg/shell/nav.tsx

                if (begin < 0)
                    begin = all.length - 1;
                all[begin].focus();
            } else {
                let i = all.findIndex(item => item === cur);
                i += step;
                if (i < 0 || i >= all.length)
-                    document.querySelector("#" + sel + " .pf-v5-c-text-input-group__text-input").focus();
+                    document.querySelector<HTMLElement>("#" + sel + " .pf-v5-c-text-input-group__text-input")?.focus();


This added line is not executed by any test.

cockpituous · 2024-12-12T08:07:06Z

pkg/shell/nav.tsx

+                                            if (g.action)
+                                                this.props.jump(g.action.target);


These 2 added lines are not executed by any test.

cockpituous · 2024-12-12T08:07:06Z

pkg/shell/nav.tsx

@@ -366,6 +412,7 @@
            target: {
                host: current_machine.address,
                path: current_machine_manifest_items.items.apps.path,
+                hash: "/",


This added line is not executed by any test.

We could add a test for this. This is about the "Edit" button in the "Applications" section. It only appears when at least one applications is installed.

It also smells like a bug fix which would be better to split out into a new commit.

cockpituous · 2024-12-12T08:07:06Z

pkg/shell/shell-modals.tsx

    }

-    const manifest = cockpit.manifests.shell || { };
+    const manifest = (cockpit.manifests.shell || { }) as ShellManifest;


This added line is not executed by any test.

cockpituous · 2024-12-12T08:07:06Z

pkg/shell/shell-modals.tsx

@@ -140,8 +144,9 @@
                        <MenuList>
                            {
                                (() => {
-                                    const filteredLocales = Object.keys(manifest.locales || {})
-                                            .filter(key => !searchInput || manifest.locales[key].toLowerCase().includes(searchInput.toString().toLowerCase()));
+                                    const locales = manifest.locales || {};


This added line is not executed by any test.

cockpituous · 2024-12-12T08:07:06Z

pkg/shell/topnav.tsx

                {cockpit.format(_("$0 documentation"), this.state.osRelease.NAME)}
            </DropdownItem>);

+        const shell_manifest = (cockpit.manifests.shell || {}) as ShellManifest;


This added line is not executed by any test.

cockpituous · 2024-12-12T08:07:06Z

pkg/shell/topnav.tsx

        // global documentation for cockpit as a whole
-        (cockpit.manifests.shell?.docs ?? []).forEach(doc => {
+        (shell_manifest.docs ?? []).forEach(doc => {


This added line is not executed by any test.

mvollmer · 2024-12-13T11:23:09Z

pkg/shell/util.tsx

@@ -91,6 +91,16 @@ export interface ManifestItem {
    keywords: ManifestKeyword[];
 }

+export interface ManifestParentSection {


Hmm... we could make some functions like manifest_parent_section(m: Manifest): ManifestParentSection which would ide all the "unsafe" JSON manipulations.

The whole Manifest stuff should go to its own file, actually.

Like, helpers which do type validation? what would they do on errors then?

This feels simultaneously too strict (hard to handle errors at that level, as these are effectively user-provided files) and too specific (we parse JSON everywhere, and should generalize the mechanics).

Are you aware of the bridge's jsonutil? That may be a more adequate way to parse JSON with strict typing, and the API would then also include default values.

martinpitt

Thanks! Nice that this found some bugs! Most of the comments are nice, bikeshedding, praise, and questions. But I have serious concerns about using as on externally read data, that feels just wrong and dangerous to me.

martinpitt · 2024-12-13T11:38:58Z

pkg/shell/util.tsx

@@ -91,6 +91,16 @@ export interface ManifestItem {
    keywords: ManifestKeyword[];
 }

+export interface ManifestParentSection {


Like, helpers which do type validation? what would they do on errors then?

This feels simultaneously too strict (hard to handle errors at that level, as these are effectively user-provided files) and too specific (we parse JSON everywhere, and should generalize the mechanics).

Are you aware of the bridge's jsonutil? That may be a more adequate way to parse JSON with strict typing, and the API would then also include default values.

pkg/shell/util.tsx

martinpitt · 2024-12-13T11:44:54Z

pkg/shell/util.tsx

+            const manifest_section = (manifest[section] || {}) as ManifestSection;
+            Object.entries(manifest_section).forEach(([prop, info]) => {


This cast is weird and IMHO detrimental. This isn't a case of "ts can't figure it out", it's literally "this could be anything as it's an user-provided (or at least user-customizable) external file on disk". So even after the as you can in no way be sure that you actually have a ManifestSection object, and the type checker will hide errors such as "component is actually an int". I.e. you'll get runtime errors because you forgot to check the type. That's the opposite of what typing should achieve.

That's why I think for this case something like json_get_object(manifest, section, {}) would be much cleaner, more robust, and also more general here. The original code was actually fine already, but the helper functions could produce proper console warnings that help admins to figure out why their manifest customizations are being ignored.

This cast is weird and IMHO detrimental.

Yes... it is keeping the status quo, but we should try to do better eventually, agreed.

I.e. you'll get runtime errors because you forgot to check the type. That's the opposite of what typing should achieve.

Hmm, I would call this input validation rather than static typing. Currently, we validate manifest input by just pretending it's all valid and let the shell crash when it isn't. (And we have been fine with that since forever.) Static typing makes this obvious, but by itself is not enough to improve it.

We would have to write new code to do real input validation. I was hoping there is a way to get TypeScript to do that. Here are some good pointers: https://stackoverflow.com/questions/33800497/check-if-an-object-implements-an-interface-at-runtime-with-typescript

Hmm, I would call this input validation rather than static typing

Yes, exactly. And input validation is a runtime thing, the type checker can't help with that. We either need some "JSON schema validation" thing, or just pick out the values individually with the expected type and default (which I personally favor -- it's explicit, tsc can help us get it right, and we don't reject manifests with wrong keys wholesale, which would be a behaviour changes).

martinpitt · 2024-12-13T11:46:52Z

pkg/shell/util.tsx

-            if (comp && comp.parent && comp.parent.component)
-                component = comp.parent.component as string;
+            if (comp && comp.parent) {
+                const parent = comp.parent as ManifestParentSection;
+                if (parent.component)
+                    component = parent.component;
+            }


In the same spirit as above, I think componennt = comp?.parent?.component would be both cleaner and more robust. Alternatively, wrap this into some json_get_object() helper with warning messages -- i.e. "nonexisting" is fine (default null) but "exists but wrong type" should warn.

So you are asking for "The Shell should validate the schema of all manifests". That's a good idea. We could have done that without TypeScript all these years, but we didn't. A bug in a manifests is like a bug in JavaScript, and should be found and fixed by the developer.

And while TypeScript might benefit from it once schema validation is done (think generating TS types from JSON schemas), I don't think TypeScript by itself is gonna help. All it does it point out our sins...

So, can I ask to keep the manifest schema validation yak out of this PR? :-)

So, can I ask to keep the manifest schema validation yak out of this PR?

I am fine with that. I am not fine with pretending that everything is good here and papering over the issue with as -- it really isn't, and tsc rightfully complains. So if it's too hard to fix these places, then let's add something greppable and obvious like @ts-expect-error. (Because as does have valid use cases, and isn't greppable)

Summary from meeting: My gut feeling/slight preference is the get_json_string() kind of accessor on usage, but I agree that for common keys we may rather want some more centralized "schema validation" thing, with a constructor. My minimum requirement here would be to add a // HACK: unsafe "as" or similar so that we can find these again.

pkg/shell/nav.tsx

martinpitt · 2024-12-13T12:05:38Z

pkg/shell/topnav.tsx

+    superuser_connection: cockpit.DBusClient | null = null;
+    superuser: cockpit.DBusProxy | null = null;
+
+    handleClickOutside = () => this.setState({ menuOpened: false, docsOpened: false });


Defining this outside of the ctor at the class level looks weird. What would this even be here? I suppose the this gets resolved at call time even for an arrow, but then wouldn't that be the this of the call environment instead of the TopNav instance?

The tests prove it works -- my eyes just trip over it. JS has too much magic!

martinpitt · 2024-12-13T12:08:55Z

pkg/shell/topnav.tsx

-        if (this.state.osRelease.DOCUMENTATION_URL)
-            docItems.push(<DropdownItem key="os-doc" to={this.state.osRelease.DOCUMENTATION_URL} target="blank" rel="noopener noreferrer" icon={<ExternalLinkAltIcon />}>
+        if (this.state.osRelease?.DOCUMENTATION_URL)
+            docItems.push(<DropdownItem key="os-doc" to={this.state.osRelease.DOCUMENTATION_URL as string} target="blank" rel="noopener noreferrer" icon={<ExternalLinkAltIcon />}>


I could tolerate this, as runtime errors here aren't catastrophic. But DOC_URL is optional and can well be undefined (or someone could valalize their os-release to be an object or int). This really shouldn't become a pattern, something like json_get_str() would be much better here.

martinpitt · 2024-12-13T12:10:12Z

pkg/shell/topnav.tsx

                {cockpit.format(_("$0 documentation"), this.state.osRelease.NAME)}
            </DropdownItem>);

+        const shell_manifest = (cockpit.manifests.shell || {}) as ShellManifest;


This is the same "unvalidated user-provided document" issue as above. We cannot claim that this is a valid ShellManifest. The typechecker will then falsely claim that our code is correct, while it is missing runtime type checks.

martinpitt · 2024-12-13T12:12:09Z

pkg/shell/topnav.tsx

+                                        this.setState((prevState: TopNavState) => {
+                                            return { docsOpened: !prevState.docsOpened };
+                                        });


btw, => ({ key: value }) is a nicer way to return an object in an arrow. Avoids the extra { return }; wrapper.

martinpitt · 2024-12-13T12:14:31Z

pkg/shell/shell-modals.tsx

-    const manifest = cockpit.manifests.shell || { };
+    const manifest = (cockpit.manifests.shell || { }) as ShellManifest;


mvollmer added 5 commits December 11, 2024 21:04

shell: Improve manifest types

97d27a5

A manifest is now just a JsonObject, and users are expected to type-cast it (or parts) of it to more concrete types, like the new ShellManifest and ManifestParentSection.

shell: Fully type "nav"

de229c6

shell: Fully type "topnav"

203a0fa

shell: Check types in "shell-modals"

b9239a0

shell: Fix two bugs found by TypeScript

35a2d1e

The cockpit.spawn function takes only two arguments (unlike cockpit.script). Location.reload() doesn't take any arguments, the "force_reload" argument is a Firefox extension.

cockpituous reviewed Dec 12, 2024

View reviewed changes

mvollmer marked this pull request as ready for review December 12, 2024 13:14

mvollmer mentioned this pull request Dec 12, 2024

shell: More types than ever #21426

Draft

1 task

mvollmer commented Dec 13, 2024

View reviewed changes

martinpitt requested changes Dec 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shell: Even more types #21425

shell: Even more types #21425

mvollmer commented Dec 12, 2024

mvollmer commented Dec 12, 2024

cockpituous Dec 12, 2024

cockpituous Dec 12, 2024

cockpituous Dec 12, 2024

mvollmer Dec 12, 2024

martinpitt Dec 13, 2024

cockpituous Dec 12, 2024

cockpituous Dec 12, 2024

cockpituous Dec 12, 2024

cockpituous Dec 12, 2024

mvollmer Dec 13, 2024

mvollmer Dec 13, 2024

martinpitt Dec 13, 2024

martinpitt left a comment

martinpitt Dec 13, 2024

martinpitt Dec 13, 2024

mvollmer Dec 13, 2024

martinpitt Dec 13, 2024

martinpitt Dec 13, 2024

mvollmer Dec 13, 2024

martinpitt Dec 13, 2024

martinpitt Dec 13, 2024

martinpitt Dec 13, 2024

martinpitt Dec 13, 2024

martinpitt Dec 13, 2024

martinpitt Dec 13, 2024

martinpitt Dec 13, 2024

		const manifest_section = (manifest[section] \|\| {}) as ManifestSection;
		Object.entries(manifest_section).forEach(([prop, info]) => {

		const manifest = cockpit.manifests.shell \|\| { };
		const manifest = (cockpit.manifests.shell \|\| { }) as ShellManifest;

shell: Even more types #21425

Are you sure you want to change the base?

shell: Even more types #21425

Conversation

mvollmer commented Dec 12, 2024

mvollmer commented Dec 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment